feat(reliability): add retry governor controls and telemetry - #40

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/development/CONFIG_FIELDS.md`:
- Around line 65-66: Update the docs entry for retryAllAccountsAbsoluteCeilingMs
(and the related env var CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS) to explicitly
state the unit "ms", the allowed bounds "0–24h" and that "0 = unlimited" so the
documentation matches runtime and UI behavior; edit the CONFIG_FIELDS.md rows
referencing retryAllAccountsAbsoluteCeilingMs (and the duplicate occurrence) to
include these details in the field description.

In `@docs/reference/settings.md`:
- Around line 89-90: Update the settings reference entry for
retryAllAccountsAbsoluteCeilingMs to explicitly state the unit ("ms") and
clarify that a value of 0 means "unlimited"; also add a brief
environment-override note showing the corresponding env var form and where it
applies (Rotation & Quota) so operators don’t have to infer units or semantics.
Make the same change for the second occurrence of
retryAllAccountsAbsoluteCeilingMs elsewhere in the document so both entries
consistently mention "ms", "0 = unlimited", and the env override behavior.

In `@index.ts`:
- Around line 2403-2430: The governor is being asked to approve a different wait
(base waitMs) than the code actually sleeps (jittered and bounded), so compute
the actual planned wait first (call addJitter(waitMs, 0.2) then apply the
ceiling/bounding logic to produce boundedWaitMs), then pass boundedWaitMs into
decideRetryAllAccountsRateLimited (instead of waitMs) and use boundedWaitMs when
updating accumulatedAllRateLimitedWaitMs and when calling sleepWithCountdown;
keep other variables (allRateLimitedRetries, retryDecision) unchanged. Add
regression tests in test/index-retry.test.ts that stub addJitter to return -20%
and +20% to assert that negative jitter still consumes remaining ceiling and
positive jitter does not allow sleeping beyond maxWaitMs.

In `@lib/codex-manager/settings-hub.ts`:
- Around line 988-996: The preview currently formats
retryAllAccountsAbsoluteCeilingMs as "0ms" but the contract treats 0 as
unlimited; update the preview-rendering logic that uses
retryAllAbsoluteCeilingMs / retryAllAbsoluteCeilingOption so that when the
resolved value is 0 it displays "unlimited" (or "Unlimited") instead of "0ms"
(apply the same change to the neighboring preview at lines 1014-1015), and add a
vitest regression that asserts this branch in test/settings-hub-utils.test.ts
(covering the case where config.retryAllAccountsAbsoluteCeilingMs === 0) so the
formatting change is exercised.

In `@lib/config.ts`:
- Around line 595-601: The function getRetryAllAccountsAbsoluteCeilingMs
currently only enforces a minimum of 0 when calling resolveNumberSetting,
allowing values above the documented 24h upper bound; update the call to
resolveNumberSetting to include a max of 24 * 60 * 60 * 1000 (24h in ms) so
runtime/env overrides are clamped, keeping the same env var
CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS and
PluginConfig.retryAllAccountsAbsoluteCeilingMs name; additionally add a vitest
regression that sets the env var CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS to a
value larger than 24h and asserts getRetryAllAccountsAbsoluteCeilingMs returns
the 24h cap.

In `@test/codex-manager-cli.test.ts`:
- Around line 1375-1379: The assertion for retryAllAccountsAbsoluteCeilingMs is
too loose; capture the pre-bump value (e.g.,
oldRetryAllAccountsAbsoluteCeilingMs) before exercising the code that triggers
the ceiling bump, then replace expect.any(Number) with a concrete postcondition:
assert that retryAllAccountsAbsoluteCeilingMs is either equal to old +
EXPECTED_RETRY_BUMP_MS (use a test constant EXPECTED_RETRY_BUMP_MS) or at
minimum greater than old
(expect(value).toBeGreaterThan(oldRetryAllAccountsAbsoluteCeilingMs)); reference
the existing property name retryAllAccountsAbsoluteCeilingMs and update the test
to compare new vs old deterministically rather than using expect.any(Number).

In `@test/index-retry.test.ts`:
- Around line 243-244: The test currently strips ANSI/control chars using a
regex on the metrics string (variable plainMetrics) which violates the
noControlCharactersInRegex lint rule; replace that logic by importing
stripVTControlCharacters from "node:util" and call
stripVTControlCharacters(metrics) (or assign to plainMetrics) instead of
String(metrics).replace(...), and update the test to use the imported helper so
linting passes and control characters are removed cleanly.

In `@test/plugin-config.test.ts`:
- Around line 954-958: The test only verifies the happy-path env override for
getRetryAllAccountsAbsoluteCeilingMs; add regression cases that assert invalid
env values fall back or clamp per the parsing logic in lib/config.ts (around the
ceiling parsing). Specifically add tests calling
getRetryAllAccountsAbsoluteCeilingMs with a PluginConfig (type PluginConfig) and
set process.env.CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS to negative values
(e.g. "-1000") and non-numeric strings (e.g. "abc") and assert the function
returns the configured default/clip value (e.g. the value from
config.retryAllAccountsAbsoluteCeilingMs or the safe clamp), and include a test
for empty string or unset env to confirm unchanged behavior; keep tests
deterministic using vitest and avoid mocking secrets.

In `@test/retry-governor.test.ts`:
- Around line 95-126: Add a deterministic regression test to cover the boundary
case where accumulatedWaitMs + waitMs === absoluteCeilingMs so the function
decideRetryAllAccountsRateLimited permits the retry; create a new vitest it(...)
case similar to the existing tests that passes parameters (e.g.,
accumulatedWaitMs: 1_000, waitMs: 1_000, absoluteCeilingMs: 2_000) and asserts
the result is { shouldRetry: true, reason: "allowed" } to ensure the equality
boundary is handled as implemented in lib/request/retry-governor.ts:68.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/index-retry.test.ts`:
- Around line 220-339: Add a deterministic concurrent-request isolation
regression in test/index-retry.test.ts that exercises two overlapping
sdk.fetch(...) calls so both enter retry handling simultaneously and verifies
per-request retry budgets aren’t shared; specifically, create two fetchPromise
variables from sdk.fetch, advance vi timers to cause both to wait/retry (use
vi.useFakeTimers(), vi.advanceTimersByTimeAsync, and control Math.random as
needed), await both promises, assert globalThis.fetch was called the expected
number of times for each request, and check
plugin.tool["codex-metrics"].execute() output contains separate "Retry governor
stops (...)" counts showing isolation; place the new test alongside the existing
retry tests and follow existing patterns for creating plugin via
OpenAIAuthPlugin and getAuth.

---

Duplicate comments:
In `@index.ts`:
- Around line 2404-2448: The code pre-bounds plannedWaitMs against
retryAllAccountsAbsoluteCeilingMs before calling
decideRetryAllAccountsRateLimited, which prevents the governor from ever
returning the "absolute-ceiling-exceeded" reason; change the logic so the
decision sees the original intended wait (jitteredWaitMs) and the absolute
ceiling separately: pass jitteredWaitMs (not the pre-bounded plannedWaitMs) into
decideRetryAllAccountsRateLimited along with retryAllAccountsAbsoluteCeilingMs
and accumulatedAllRateLimitedWaitMs, then only clamp/planned-execute the actual
sleep after the decision (or explicitly record an "absolute-ceiling-exceeded"
stop via recordRetryGovernorStopReason if accumulated+planned would exceed the
ceiling). Ensure references: plannedWaitMs, jitteredWaitMs,
decideRetryAllAccountsRateLimited, retryAllAccountsAbsoluteCeilingMs,
accumulatedAllRateLimitedWaitMs, and recordRetryGovernorStopReason are updated
accordingly.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/index-retry.test.ts`:
- Around line 220-368: Add a deterministic test that forces the retry-limit stop
path by setting CODEX_AUTH_RETRY_ALL_MAX_RETRIES to "0" (or another value that
yields zero retries), then use OpenAIAuthPlugin + auth.loader to call sdk.fetch
and assert it returns 429 and globalThis.fetch was not called; finally call
plugin.tool["codex-metrics"].execute() and assert the metrics string contains
"Retry governor stops (retry limit): 1" so the counter (from the retry-governor
logic referenced in lib/request/retry-governor.ts) is exercised and reported.

---

Duplicate comments:
In `@test/index-retry.test.ts`:
- Around line 220-368: Add a deterministic concurrent-request test that verifies
per-request retry-budget isolation by calling plugin.auth.loader(... ) to get
sdk and issuing multiple overlapping sdk.fetch(...) calls (e.g., start two
fetches before advancing timers), mock Math.random and timers via vi to keep
waits deterministic, then advance timers and assert that each fetch completes
independently (globalThis.fetch called expected times) and
plugin.tool["codex-metrics"].execute() reports separate stop counts only for the
requests that hit their per-request ceilings (use env vars like
CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS / CODEX_AUTH_RETRY_ALL_MAX_WAIT_MS and
CODEX_AUTH_RETRY_ALL_MAX_RETRIES to control behavior); ensure the test uses
vitest APIs (vi.spyOn, vi.advanceTimersByTimeAsync) and does not rely on shared
state so retry accounting races would fail if budgets were global.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/index-retry.test.ts`:
- Around line 306-334: The test currently only triggers one retry because
mockInitialNullCalls remains at its default; before importing OpenAIAuthPlugin
in this test, set mockInitialNullCalls = 2 so the loader will return two initial
null auths and exercise both bounded waits; keep the environment vars
(CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS and CODEX_AUTH_RETRY_ALL_MAX_RETRIES)
as-is and ensure Math.random is mocked to 0 so the -20% jitter path is used,
then proceed to create the plugin via OpenAIAuthPlugin and assert fetch is
called after advancing timers by the full ceiling.

---

Outside diff comments:
In `@test/index-retry.test.ts`:
- Around line 23-35: The mock disables token-refresh races by hardcoding
shouldRefreshToken to false and making refreshAndUpdateToken a no-op; restore
race coverage by changing the mock: implement shouldRefreshToken to return true
for an expired-token sentinel (or based on a controllable test flag) and
implement refreshAndUpdateToken as an async function that waits (to allow
overlapping requests), updates a shared mock auth token, and returns the updated
auth so the existing overlapping-request regression tests (those exercising
retry behavior) can detect queueing races; additionally add a deterministic unit
test variant that injects Windows-style path edge-case inputs (backslashes,
drive letters, trailing separators) into the retry/index logic to reproduce
filesystem edge regressions—use the existing test helpers and vitest
timers/mocks to keep tests deterministic.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/request/retry-governor.ts`:
- Around line 54-73: The ceiling check allows a retry when accumulatedWaitMs
equals absoluteCeilingMs (and plannedWaitMs is 0), causing a zero-delay spin;
change the absolute ceiling check in retry-governor (use >= instead of >) so the
condition reads: if (absoluteCeilingMs > 0 && accumulatedWaitMs +
ceilingCheckWaitMs >= absoluteCeilingMs) return { shouldRetry: false, reason:
"absolute-ceiling-exceeded" }; update the logic around ceilingCheckWaitMs
(computed from accumulatedWaitMs, waitMs, plannedWaitMs) only if needed to
preserve the same semantics, and add deterministic vitest regression tests in
test/retry-governor.test.ts and test/index-retry.test.ts that reproduce the
exhausted-ceiling path (do not mock real secrets, ensure tests are deterministic
across concurrency and Windows FS).